-
Notifications
You must be signed in to change notification settings - Fork 845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement spec change to only accept Context as span parent. #1611
Implement spec change to only accept Context as span parent. #1611
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1611 +/- ##
============================================
+ Coverage 85.36% 86.38% +1.02%
+ Complexity 1382 1378 -4
============================================
Files 164 162 -2
Lines 5418 5349 -69
Branches 556 554 -2
============================================
- Hits 4625 4621 -4
+ Misses 594 521 -73
- Partials 199 207 +8 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this is conflating two unrelated ideas, the parent / child relationship of spans (a logical connection of IDs) and a mechanism for propagating data within a process, often across threads (Context). The end result is contexts have spans, and spans also have contexts - it's not quite circular but it's not a logical relationship I think.
If someone needs to propagate data along with the span, there's no need to do it with as part of parent/child relationship I think, the data can be added to the Context next to the span, e.g., after creating the span, Context.withValue(valuetopropagate, Context.current())
. If we really need a simpler API for adding arbitrary information to go along with the span (that is how I'm reading the rationale in #875), we could have a first-class field for extra data like Brave. But I find it confusing if we try to use the span parent for this sort of data.
sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java
Outdated
Show resolved
Hide resolved
sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/data/SpanData.java
Outdated
Show resolved
Hide resolved
Please, can we discuss such fundamental design concerns on the spec PR open-telemetry/opentelemetry-specification#875? |
I realized that context APIs don't allow iterating over the context, so my concern of leaking unrelated information into the exporter isn't really a concern. So still about lifecycle, how about an example with the root span. Want to propagate some information with it.
I want to add data to the span, so I have to add it to the context. And I add a parent even though it's a root span. This API seems very confusing to me. Perhaps builder has helpers to set the values, but then this would create an internal context forked from current when building the span that probably can't be mounted. I think we would have to change |
Doh sorry wrong PR! |
…rent Conflicts: api/src/main/java/io/opentelemetry/trace/DefaultTracer.java
…rent Conflicts: exporters/jaeger/src/test/java/io/opentelemetry/exporters/jaeger/AdapterTest.java exporters/zipkin/src/test/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporterEndToEndHttpTest.java exporters/zipkin/src/test/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporterTest.java sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java sdk/tracing/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java testing_internal/src/main/java/io/opentelemetry/sdk/trace/TestSpanData.java
@Oberon00 What would you think about narrowing the scope of this PR to only update the API, rather than also change it to move the Context all the way down into the exporter layer? I think your spec issue has had the scope narrowed recently to only include the API changes. |
Indeed, this was changed in today's SIG spec meeting. I will update the PR (will probably remove more than half the changes) but I didn't get around to it yet. |
I updated the PR. Currently it is even more minimal than the spec PR (no Context parameter for OnStart), but that would be trivial follow-up. |
thanks! |
This is now ready to merge, since the spec PR was merged. Note that a follow-up is required to add the new Context parameter for SpanProcessor.OnStart. |
@@ -32,8 +33,8 @@ | |||
void doNotCrash_NoopImplementation() { | |||
Span.Builder spanBuilder = tracer.spanBuilder("MySpanName"); | |||
spanBuilder.setSpanKind(Kind.SERVER); | |||
spanBuilder.setParent(DefaultSpan.getInvalid()); | |||
spanBuilder.setParent(DefaultSpan.getInvalid().getContext()); | |||
spanBuilder.setParent(TracingContextUtils.withSpan(DefaultSpan.create(null), Context.ROOT)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
youch. this API sure is ugly! Remind me again how this is going to make things easier/better for our users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a unit test. Normally you would just call setNoParent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough, but I think having to do parenting via the TCU is going to be pretty counter-intuitive to most users. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fully agree, but at the same time, I don't think users should normally do that. Usually you would just use the implicit current parent, or sometimes you need to capture a context with Context.current()
and forward that to somewhere else. If you start a span at one thread and need to continue it at another thread while not setting it as active in the current thread is the only place you would need TracingContextUtils. I also think there should be some may to make that more easy. I could imagine span.inCurrentContext()
(Java 8 default methods?), or TracingContextUtils.spanInCurrent(span)
to make this easier.
Remind me again how this is going to make things easier/better for our users?
As you said, using TracingContextUtils is rather unintuitive, and thus I think most users did not do it, they just passed around spans without the remaining context. However, a custom propagator should be able to set a value in the context in extract and rely on it still being available in inject.
BTW, using inject
is a place where you already have to often muck around with TracingContextUtils currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to assume the implicit current parent is accurate?
For instance, when dealing with asynchronous methods the thread being executed on is usually not the same thread that might have initiated/received the original request. In that situation, the implicit current parent from ThreadLocal
is more than likely not the parent you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can provide some convenience function (default interface implementation?), so one can do Context capturedParent = span.withCurrentContext()
. But it would be interesting how common the cases where you would need this even are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, it's a lot messier, but more importantly way less obvious that that's what is needed to achieve the desired result.
With respect to a default interface implementation, that wouldn't be possible with the JDK 7 requirement, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(FWIW, we need more convenience functions in TracingContextUtils
, or in whatever new name it ends up having ;) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kenfinnigan Java 7 was dropped recently, Java 8 is now required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Oberon00, don't follow things for a few weeks, and everything changes!
...pentelemetry/sdk/extensions/trace/testbed/concurrentcommonrequesthandler/RequestHandler.java
Outdated
Show resolved
Hide resolved
opentracing_shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java
Outdated
Show resolved
Hide resolved
…rent Conflicts: sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Oberon00!
Partial implementation of (now merged) open-telemetry/opentelemetry-specification#875
This is a prototype for spec PR open-telemetry/opentelemetry-specification#875 (spec issue open-telemetry/opentelemetry-specification#510).
To be more minimal, it does not currently add the Context parameter to SpanProcessor.OnStart, but that would be a trivial follow-up.
OLD OUTDATED DESCRIPTION
It replaces the parentSpanId + hasRemoteParent in RecordEventsReadableSpan with a full io.grpc.Context.
That was the simple part. The part that is a bit funky is that DefaultSpan now has also has a parent Context in addition to still having a SpanContext for itself.All in all, this removes 24 lines according to
git diff --stat
, so as stated in the spec PR, the total complexity does not really increase here.The most interesting changes are in SpanBuilderSdk, RecordEventsReadableSpan
and DefaultSpan.END OLD OUTDATED DESCRIPTION